THRIFT-6045: Add Ruby recursion depth limit by kpumuk · Pull Request #3691 · apache/thrift

kpumuk · 2026-07-31T22:22:21Z

Ruby struct, union, and exception serialization previously had no recursion-depth limit, allowing deeply nested or cyclic values to recurse until the Ruby stack was exhausted.

This adds a call-local remaining-depth budget of 64 to the pure-Ruby and native read/write paths. Nested structs, unions, and exceptions consume the budget while containers pass it through unchanged; unknown and mismatched fields remain independently bounded by the existing protocol skip limit. The optional depth arguments and the migration required for custom exact-arity serialization overrides are documented for Ruby 0.25.0.

Did you create an Apache Jira ticket? (THRIFT-6045)
If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
Did you squash your changes to a single commit? (not required, but preferred)
Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (2)

lib/rb/lib/thrift/exceptions.rb:21

Thrift::ApplicationException#read/#write references Types::..., but this file doesn’t require thrift/types. If a consumer requires thrift/exceptions directly (or gets it indirectly), calling these methods can raise NameError: uninitialized constant Thrift::Types unless thrift/types was loaded elsewhere. Adding the explicit require here makes the file self-contained.

module Thrift
  DEFAULT_RECURSION_DEPTH = 64

lib/rb/lib/thrift/protocol/base_protocol.rb:23

BaseProtocol uses Types::... extensively (e.g., in write_type / read_type), but this file doesn’t require thrift/types. Since it now requires thrift/exceptions, it’s more likely to be loaded directly; adding an explicit require 'thrift/types' avoids NameError when base_protocol is required standalone.

# this require is to make generated struct definitions happy
require 'set'
require 'thrift/exceptions'

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

lib/rb/lib/thrift/protocol/base_protocol.rb:253

BaseProtocol#write_field always calls write_type with a third argument (remaining_depth), even when it is nil. That changes the call arity from 2→3 for the common case and will break custom protocol implementations that override write_type(field_info, value) with the previous 2-arg signature.

To preserve backward compatibility, only pass remaining_depth when it is non-nil (i.e., when the caller is explicitly propagating a depth budget).

      write_field_begin(field_info[:name], field_info[:type], fid)
      write_type(field_info, value, remaining_depth)
      write_field_end

Jens-G · 2026-08-02T12:07:23Z

Code review

No blocking issues. Depth accounting is symmetric between the pure-Ruby and the native path (both admit exactly 64 struct levels and raise on the 65th), containers pass the budget through unchanged on every path, and the guards added by THRIFT-6025 (negative/oversized container sizes), THRIFT-6104 (Set subclasses), THRIFT-6124 (reset of reused deserialization targets) and THRIFT-6013 (BaseProtocol#skip's independent bound) all survive the refactor.

Suggestions, none blocking:

The new depth check runs before the reused-target reset that THRIFT-6124 (d1d7280c3) made the unconditional first action of read. reused_obj.read(prot, 0) raises DEPTH_LIMIT with the previous deserialization's field values still in place, which breaks the invariant serializer_spec.rb's "does not retain previous struct state when reading fails" encodes — that test exercises EOFError, a failure mode that existed when THRIFT-6124 landed; DEPTH_LIMIT is a new one. Not reachable from a wire payload (Deserializer#deserialize always uses the default budget, and recursive descent always targets a freshly allocated object), but read(protocol, depth) is public API as of this PR. Moving the raise below the reset restores the invariant.

thrift/lib/rb/lib/thrift/struct.rb

Lines 84 to 88 in c5761b7

    
           def read(iprot, remaining_depth = DEFAULT_RECURSION_DEPTH) 
        
             raise ProtocolException.new(ProtocolException::DEPTH_LIMIT, 'Maximum recursion depth exceeded') if remaining_depth <= 0 
        
             unless instance_variables.empty? 
        
               defaults = fields_with_default_values

Same ordering in Union#read:

thrift/lib/rb/lib/thrift/union.rb

Lines 58 to 62 in c5761b7

    
           def read(iprot, remaining_depth = DEFAULT_RECURSION_DEPTH) 
        
             raise ProtocolException.new(ProtocolException::DEPTH_LIMIT, 'Maximum recursion depth exceeded') if remaining_depth <= 0 
        
             @setfield = nil 
        
             @value = nil

and in the extension, where parse_recursive_args raises before rb_thrift_struct_read_recursive's rb_check_frozen / reset_struct_field block (L664-L667) is entered:

thrift/lib/rb/ext/struct.c

Lines 711 to 716 in c5761b7

    
           // cppcheck-suppress constParameterCallback 
        
           static VALUE rb_thrift_struct_read(int argc, VALUE *argv, VALUE self) { 
        
             VALUE protocol; 
        
             int remaining_depth = parse_recursive_args(argc, argv, &protocol); 
        
             return rb_thrift_struct_read_recursive(self, protocol, remaining_depth); 
        
           }

Minor: the comment "Structs consume this call-local budget" sits on static int recursion_limit, which is file-scope and assigned once in Init_struct. It describes the remaining_depth parameter, not this variable.

thrift/lib/rb/ext/struct.c

Lines 36 to 40 in c5761b7

    
           #define IS_CONTAINER(ttype) ((ttype) == TTYPE_MAP || (ttype) == TTYPE_LIST || (ttype) == TTYPE_SET) 
        
           #define STRUCT_FIELDS(obj) rb_const_get(CLASS_OF(obj), fields_const_id) 
        
           // Structs consume this call-local budget; containers pass it through unchanged. 
        
           static int recursion_limit;

On the Init_struct thread: your reading is right. Init_thrift_native resolves Thrift, Thrift::Types and Thrift::ProtocolException::DEPTH_LIMIT unconditionally before it reaches Init_struct, so a standalone require "thrift_native" already fails earlier, and a C-side literal would only add a second copy of the default — the drift I flagged as issue 3 on the Python equivalent, #3592.

Copilot's suppressed note about write_type's 2→3 arity is worth a decision either way. If you keep the unconditional third argument, one sentence in the README migration note would help: omitting remaining_depth does not inherit the caller's budget, it restarts it at 64.

thrift/lib/rb/lib/thrift/protocol/base_protocol.rb

Lines 292 to 299 in c5761b7

    
             write_uuid(value) 
        
           when Types::STRUCT 
        
             if remaining_depth 
        
               value.write(self, remaining_depth - 1) 
        
             else 
        
               value.write(self) 
        
             end 
        
           else

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

Client: rb Co-Authored-By: OpenAI Codex (GPT-5.6) <codex@openai.com>

kpumuk · 2026-08-03T13:05:54Z

Thanks for the review. The reset-order observation is correct: I reproduced that read(protocol, 0) leaves an existing object’s values intact in both the Ruby and native implementations.

I thought about whether this should count as the kind of failed read covered by THRIFT-6124. I am leaning towards protecting an object once deserialization has started: fields are cleared before reading the new payload, so an EOFError or another protocol failure cannot expose stale values. A zero depth is slightly different because the call is rejected before the protocol is touched.

In normal use, Deserializer always starts with the default budget. When recursive reading eventually reaches zero, the target is a newly allocated child, so there is no old state to preserve. The stale-state case therefore requires application code to call read(protocol, 0) directly on an already-used object. Resetting in that case would mean mutating the receiver even though the read was rejected before it began; it would also change the error for frozen objects from DEPTH_LIMIT to FrozenError. On balance, I think keeping the depth check first gives the cleaner behavior.

I did update the C comment—the original wording described the call-local remaining_depth value but was attached to the file-level default.

The write_type arity point is also fair. This is an intentional 0.25.0 compatibility change, and the README migration note asks exact-arity custom overrides to accept and forward the optional depth. There is deliberately no hidden protocol state from which an omitted argument could inherit a caller’s budget, so omission starts a fresh default budget while recursive custom writers must pass the current one explicitly.

And thanks for confirming the conclusion on standalone thrift_native loading.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

lib/rb/README.md:184

This new behavior description is inserted immediately before the ### 0.24.0 heading but doesn’t have its own version heading, which makes it ambiguous which release it applies to. Add a ### 0.25.0 (or the intended version) heading above these lines so the changelog structure remains clear.

Ruby struct, union, and exception serialization methods now accept an optional
remaining struct depth: `read(protocol, depth = 64)` and
`write(protocol, depth = 64)`.
Unknown and mismatched fields remain independently bounded by
`Thrift::BaseProtocol#skip`. The `write_field` and `write_type` helpers accept
an optional `remaining_depth`: custom writers must use
`write_field(field_info, fid, value, remaining_depth)` or
`write_type(field_info, value, remaining_depth)` for struct values. Custom
protocols and custom struct, union, or exception serialization overrides with
exact argument counts must accept and forward the optional depth argument (or
use `*args`) before upgrading.

### 0.24.0

lib/rb/lib/thrift/struct.rb:86

The error message string 'Maximum recursion depth exceeded' is duplicated across multiple Ruby files (and also in the C extension). To keep messages consistent and simplify future changes/localization, consider defining a single constant (e.g., under Thrift) for the message and referencing it from struct/union/exceptions and the native extension.

    def read(iprot, remaining_depth = DEFAULT_RECURSION_DEPTH)
      raise ProtocolException.new(ProtocolException::DEPTH_LIMIT, 'Maximum recursion depth exceeded') if remaining_depth <= 0

Copilot AI review requested due to automatic review settings July 31, 2026 22:22

This comment was marked as spam.

Sign in to view

mergeable Bot added the ruby Pull requests that update Ruby code label Jul 31, 2026

kpumuk requested a review from Jens-G July 31, 2026 22:24

kpumuk force-pushed the rb-recursion-depth branch from f6b4ee2 to 8af6280 Compare August 1, 2026 00:07

Copilot AI review requested due to automatic review settings August 1, 2026 00:07

Copilot started reviewing on behalf of kpumuk August 1, 2026 00:07 View session

kpumuk force-pushed the rb-recursion-depth branch from 8af6280 to c5761b7 Compare August 1, 2026 00:12

Copilot AI reviewed Aug 1, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 1, 2026 00:13

Copilot started reviewing on behalf of kpumuk August 1, 2026 00:14 View session

Copilot AI reviewed Aug 1, 2026

View reviewed changes

Comment thread lib/rb/ext/struct.c

THRIFT-6045: Add Ruby recursion depth limit

d244a37

Client: rb Co-Authored-By: OpenAI Codex (GPT-5.6) <codex@openai.com>

kpumuk force-pushed the rb-recursion-depth branch from c5761b7 to d244a37 Compare August 3, 2026 13:02

Copilot AI review requested due to automatic review settings August 3, 2026 13:02

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Comment thread lib/rb/ext/struct.c

Copilot started reviewing on behalf of kpumuk August 3, 2026 13:59 View session

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

THRIFT-6045: Add Ruby recursion depth limit - #3691

THRIFT-6045: Add Ruby recursion depth limit#3691
kpumuk wants to merge 1 commit into
apache:masterfrom
kpumuk:rb-recursion-depth

kpumuk commented Jul 31, 2026

Uh oh!

This comment was marked as spam.

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Jens-G commented Aug 2, 2026

Uh oh!

kpumuk commented Aug 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

kpumuk commented Jul 31, 2026

Uh oh!

This comment was marked as spam.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Jens-G commented Aug 2, 2026

Code review

Uh oh!

kpumuk commented Aug 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants